Skip to content

fix(cli): align test/ error messages with 4-ingredient strategy#1259

Merged
John-David Dalton (jdalton) merged 2 commits intomainfrom
jdd/error-msg-test
Apr 24, 2026
Merged

fix(cli): align test/ error messages with 4-ingredient strategy#1259
John-David Dalton (jdalton) merged 2 commits intomainfrom
jdd/error-msg-test

Conversation

@jdalton
Copy link
Copy Markdown
Contributor

@jdalton John-David Dalton (jdalton) commented Apr 22, 2026

Summary

PR 6 of the error-message series. Covers test utilities: packages/cli/src/test/ (the Socket-JSON contract validator + auth-flow mocks, which are shared across the test suite) and packages/cli/test/ (the bash smoke harness + its JS counterpart).

~16 messages across 4 files. Full test suite (343 files / 5225 tests) still passes.

What's fixed

src/test/json-output-validation.mts (6 throws)

Socket CLI guarantees a JSON output contract: {ok: true, data: ...} on success or {ok: false, message: ...} on failure. This file's validator is the guardrail. Before, it said things like "JSON output missing required 'ok' boolean field" which didn't tell a failing test author what the full contract looked like or how to satisfy it.

Before: JSON output 'ok' should be true when exit code is 0: {full payload...}
After: Socket JSON contract violation: exit code is 0 but "ok" is false (expected true); got: {preview}... — set ok:true when the command succeeds, or return a non-zero exit code

Long payloads are now truncated to 200 chars in the message so stacktraces stay readable.

src/test/mocks/socket-auth.mts (2 throws)

throw new Error('Authentication failed') and throw new Error('OAuth timeout') — before, a test failure here looked like a real auth outage. Now both errors explicitly identify themselves as test fixtures and point at the config flag that controls them.

test/json-output-validation.mts (2 non-throwing returns)

This is a lighter shim that returns {ok: false, message: ...} instead of throwing. Updated to match the throw-path messages so a failing integration test shows the parse error and a stdout preview.

test/smoke.sh (6 label strings)

The bash smoke harness duplicates the JS validator's logic in shell. Updated its labels to mirror the new wording so the two harnesses produce consistent errors.

Tests

Nothing else needed updating — grep confirmed the "Authentication failed" and "Unknown error" hits elsewhere are unrelated (tests constructing their own new AuthError('Authentication failed') as test data, or matching on a distinct production error in src/utils/socket/api.mts).

Full suite: 5225/5225 tests pass.

Test plan

  • CI green
  • Sanity: break the JSON output of any --json command and verify the new validator message points at the contract violation clearly

Note

Low Risk
Low risk: changes are confined to test utilities and smoke harness messaging, with no production logic impact aside from clearer failures when tests detect malformed JSON or misconfigured mocks.

Overview
Tightens and standardizes test-time validation of the Socket CLI --json output contract by upgrading error messages to explicitly call out contract violations, include actionable guidance, and show a truncated stdout preview for readability.

Updates the auth-flow test mocks to throw fixture-specific failures (instead of generic auth errors), and aligns both the JS integration validator and test/smoke.sh labels with the same clearer parse/contract diagnostics.

Reviewed by Cursor Bugbot for commit a8efd4f. Configure here.

Rewrites the Socket-JSON contract validator and auth-flow mocks under
packages/cli/src/test/ and packages/cli/test/ to follow the What /
Where / Saw vs. wanted / Fix strategy from CLAUDE.md.

Sources:
- src/test/json-output-validation.mts (6 throws): each violation now
  spells out the full Socket-JSON contract, the received value, and
  a concrete fix ("add ok:true", "return empty object", etc.).
  Long output payloads are truncated to 200 chars in the message so
  errors stay readable.
- src/test/mocks/socket-auth.mts (2 throws): "Authentication failed"
  and "OAuth timeout" now call out that they come from a test
  fixture and point at the configuration flag to change.
- test/json-output-validation.mts (2 non-throwing returns): message
  values now include the exit code / parse error and a stdout
  preview so failing tests diagnose themselves.
- test/smoke.sh (6 labels): updated to mirror the TS validator so
  the bash and JS harnesses produce the same wording.

Tests: full suite (343 files / 5225 tests) still passes. No
assertions touched — the unrelated "Authentication failed" hits
in other tests are test fixtures constructing their own Errors,
not references to the mock.

Follows strategy from #1254. Continues #1255-#1258.
Switch `(e as Error).message` to `e instanceof Error ? e.message : String(e)` so that when a non-Error value is thrown (strings, objects, null) the error message stays informative instead of becoming 'undefined'.

Same fix as applied to #1260 (iocraft.mts) after Cursor bugbot flagged the pattern on that PR.
@jdalton
Copy link
Copy Markdown
Contributor Author

bugbot run

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit a8efd4f. Configure here.

@jdalton John-David Dalton (jdalton) merged commit a0ca97a into main Apr 24, 2026
14 checks passed
@jdalton John-David Dalton (jdalton) deleted the jdd/error-msg-test branch April 24, 2026 19:35
John-David Dalton (jdalton) added a commit that referenced this pull request Apr 24, 2026
…, terminal) (#1260)

* fix(cli): align utils/ miscellaneous error messages with 4-ingredient strategy

Final PR in the error-message series. Covers everything not already
touched by #1255-#1259: utils/basics, utils/config, utils/fs,
utils/git, utils/npm, utils/promise, utils/terminal, and the flags
module at the root of the CLI tree.

Sources:
- flags.mts: 2 throws (--max-old-space-size, --max-semi-space-size)
  — name the flag, show the offending value, suggest a concrete
  megabyte value.
- utils/config.mts: 1 throw (SOCKET_CLI_CONFIG base64 decode) —
  explains the replacement-character symptom and how to re-encode.
- utils/basics/vfs-extract.mts: 4 throws (SEA VFS extraction for
  Python + security tools) — name the missing paths, the exit
  codes, and point at the "rebuild the SEA binary" fix.
- utils/promise/queue.mts: 1 throw (PromiseQueue concurrency guard)
  — show the offending value and suggest 4/8.
- utils/npm/spec.mts: 1 throw (PURL conversion) — show the input,
  state what a valid npm spec looks like.
- utils/git/operations.mts: 1 throw (git-not-on-PATH) — point at
  install and the local-path env-var override.
- utils/git/gitlab-provider.mts: 2 throws (no token, PR creation
  after retries) — name the token scope, the retry count, the
  repo/head refs.
- utils/fs/path-resolve.mts: 1 throw (npm path-walk iteration cap)
  — name the start path, current directory, and what usually
  causes the cycle (symlinks).
- utils/terminal/iocraft.mts: 1 throw (native-module load failure)
  — show the underlying error and the offending platform/arch
  triple.

Skipped (already informative):
- github-provider.mts pass-through errors (forward inner CResult
  cause/message)
- gitlab-provider.mts try/catch wrappers that call
  formatErrorWithDetail (inner error has context)
- 'process.exit called' sentinel throws in npm/pnpm/yarn/with-
  subcommands paths (test harness re-raise markers, not user-facing)

Tests updated:
- test/unit/utils/promise/queue.test.mts (2 assertions)
- test/unit/utils/npm/spec.test.mts (2 assertions)
- test/unit/utils/git/gitlab-provider.test.mts (3 assertions)

Full suite (343 files / 5225 tests) passes.

Completes the series: #1255 (commands/) → #1256 (utils/dlx/) →
#1257 (utils/update + utils/command/) → #1258 (env/ + constants/) →
#1259 (test/) → this.

* fix(cli): address Cursor bugbot findings on error messages

Four issues flagged by Cursor bugbot on #1260:

1. (Medium) gitlab-provider.mts: error said 'check GL_TOKEN permissions'
   but the actual env var is GITLAB_TOKEN (as the same file's getGitLabToken
   confirms). Fixed to GITLAB_TOKEN.

2. (Medium) git/operations.mts: error suggested 'set SOCKET_CLI_GIT_PATH
   to point at a specific binary' — that env var is not read anywhere.
   Removed the false suggestion; kept the real fix (install git and put
   it on PATH) with package-manager examples.

3. (Low) terminal/iocraft.mts: '(e as Error).message' evaluates to
   undefined when a non-Error is thrown. Switched to
   'e instanceof Error ? e.message : String(e)' for safe stringification.

4. (Low) gitlab-provider.mts: error said 'after ${retries} retries' but
   the loop runs attempt 1..retries inclusive — retries is the total
   attempt count, not retries beyond the first. Reworded to 'attempts'.
   Matching test assertions updated.

Caught by #1260 bugbot review.

* chore(cli): use joinAnd + getErrorCause helpers in utils/ misc

- basics/vfs-extract.mts: missingTools list now renders as prose
  via joinAnd('a, b, and c').
- terminal/iocraft.mts: inline `e instanceof Error ? e.message :
  String(e)` swapped for getErrorCause(e). require() of a native
  binding can throw non-Error values, so the safe-stringify with
  UNKNOWN_ERROR fallback is correct here.

No behavior change for Error throws.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants